[MLIR] Erase unreachable blocks before applying patterns in the greedy rewriter #153957
Conversation
|
@llvm/pr-subscribers-mlir-arith @llvm/pr-subscribers-mlir-core Author: Mehdi Amini (joker-eph) ChangesOperations like: are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops. To avoid this we can remove unreachable blocks before attempting to apply patterns. Fixes #153732 Full diff: https://github.com/llvm/llvm-project/pull/153957.diff 2 Files Affected:
diff --git a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
index 607b86cb86315..0a2a0cc1d5c73 100644
--- a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
+++ b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
@@ -871,7 +871,18 @@ LogicalResult RegionPatternRewriteDriver::simplify(bool *changed) && {
ctx->executeAction<GreedyPatternRewriteIteration>(
[&] {
- continueRewrites = processWorklist();
+ continueRewrites = false;
+
+ // Erase unreachable blocks
+ // Operations like:
+ // %add = arith.addi %add, %add : i64
+ // are legal in unreachable code. Unfortunately many patterns would be
+ // unsafe to apply on such IR and can lead to crashes or infinite
+ // loops.
+ continueRewrites |=
+ succeeded(eraseUnreachableBlocks(rewriter, region));
+
+ continueRewrites |= processWorklist();
// After applying patterns, make sure that the CFG of each of the
// regions is kept up to date.
diff --git a/mlir/test/Dialect/Arith/canonicalize.mlir b/mlir/test/Dialect/Arith/canonicalize.mlir
index 78f67821da138..c9498b147246a 100644
--- a/mlir/test/Dialect/Arith/canonicalize.mlir
+++ b/mlir/test/Dialect/Arith/canonicalize.mlir
@@ -3363,3 +3363,18 @@ func.func @bf16_fma(%arg0: vector<32x32x32xbf16>, %arg1: vector<32x32x32xbf16>,
}
}
#-}
+
+// CHECK-LABEL: func @unreachable()
+// CHECK-NEXT: return
+// CHECK-NOT: arith
+ func.func @unreachable() {
+ return
+ ^unreachable:
+ %c1_i64 = arith.constant 1 : i64
+ // This self referencing operation is legal in an unreachable block.
+ // Many patterns are unsafe with respect to this kind of situation,
+ // check that we don't infinite loop here.
+ %add = arith.addi %add, %c1_i64 : i64
+ cf.br ^unreachable
+ }
+}
\ No newline at end of file
|
joker-eph
force-pushed
the
unreachable_canonicalize
branch
from
c1dd74b to
177c693
Compare
…y rewriter
Operations like:
%add = arith.addi %add, %add : i64
are legal in unreachable code. Unfortunately many patterns would be unsafe to
apply on such IR and can lead to crashes or infinite loops.
To avoid this we can remove unreachable blocks before attempting to apply
patterns.
We may have to do this also whenever the CFG is changed by a pattern, it is left
up for future work right now.
Fixes llvm#153732
joker-eph
force-pushed
the
unreachable_canonicalize
branch
from
177c693 to
d2502c2
Compare
| // This self referencing operation is legal in an unreachable block. | ||
| // Many patterns are unsafe with respect to this kind of situation, | ||
| // check that we don't infinite loop here. | ||
| %add = arith.addi %add, %c1_i64 : i64 |
There was a problem hiding this comment.
Doesn't an SSACFG region require that all definitions come before their uses?
There was a problem hiding this comment.
Not in unreachable blocks where self-references are allowed.
There was a problem hiding this comment.
What's the reason why we allow this? Is it also allowed to use values that are going to be defined later?
There was a problem hiding this comment.
It is because an unreachable block is considering to self-dominate itself: so there is no violation of the dominance.
This isn't an MLIR thing by the way, that's how LLVM works already.
One of the reason why this is really useful is that it preserves the ability to perform local transformation without having to look for "effects at a distance".
That is: simple patterns of replacement doing RAUW, or updating the CFG can easily lead to this situation.
There was a problem hiding this comment.
Interesting. The moment a block is unreachable, we completely turn off all dominance checking. It's basically like a graph region from that point on. This IR also verifies:
func.func @unreachable() {
return
^unreachable:
%add = arith.addi %add, %c1_i64 : i64
%c1_i64 = arith.constant 1 : i64
cf.br ^unreachable
}One of the reason why this is really useful is that it preserves the ability to perform local transformation without having to look for "effects at a distance".
I don't follow. When you do a local transformation, you don't know that you are operating on unreachable IR. RAUW etc. must be guarded with extra checks to ensure that no dominance violations are created.
There was a problem hiding this comment.
I don't follow. When you do a local transformation, you don't know that you are operating on unreachable IR.
Exactly, so you don't know you would violate things (like creating a self-referencing operation).
For example the "self-referencing operation can be created from folding the second add here:
^unreachable(%arg : i64):
%add = arith.addi %add_plus_zero, %c1_i64 : i64
%add_plus_zero = arith.addi %arg, %c0_i64 : i64
cf.cond_br %cond ^unreachable, ^other
The folder would just do RAUW here.
Of course no one builds the IR that way, but you start with a CFG which has blocks which are dynamically unreachable, then you propagate some constants, simplify some branches, end up with some unreachable blocks, and after more simplification you end up here.
Kind of: I'm working on a separate patch to the walk-and-apply driver to skip unreachable code instead. |
|
Here is the patch to fix the walk-and-apply driver: #154038 |
…river This is similar to the fix to the greedy driver in #153957 ; except that instead of removing unreachable code, we just ignore it. Operations like: %add = arith.addi %add, %add : i64 are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops.
…river This is similar to the fix to the greedy driver in #153957 ; except that instead of removing unreachable code, we just ignore it. Operations like: %add = arith.addi %add, %add : i64 are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops.
…river This is similar to the fix to the greedy driver in #153957 ; except that instead of removing unreachable code, we just ignore it. Operations like: %add = arith.addi %add, %add : i64 are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops.
…river (#154038) This is similar to the fix to the greedy driver in #153957 ; except that instead of removing unreachable code, we just ignore it. Operations like: ``` %add = arith.addi %add, %add : i64 ``` are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops.
…y rewriter (llvm#153957) Operations like: %add = arith.addi %add, %add : i64 are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops. To avoid this we can remove unreachable blocks before attempting to apply patterns. We may have to do this also whenever the CFG is changed by a pattern, it is left up for future work right now. Fixes llvm#153732
…river (llvm#154038) This is similar to the fix to the greedy driver in llvm#153957 ; except that instead of removing unreachable code, we just ignore it. Operations like: ``` %add = arith.addi %add, %add : i64 ``` are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops.
…y rewriter (#153957) Operations like: %add = arith.addi %add, %add : i64 are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops. To avoid this we can remove unreachable blocks before attempting to apply patterns. We may have to do this also whenever the CFG is changed by a pattern, it is left up for future work right now. Fixes #153732
…river (#154038) This is similar to the fix to the greedy driver in #153957 ; except that instead of removing unreachable code, we just ignore it. Operations like: ``` %add = arith.addi %add, %add : i64 ``` are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops.
4 participants
Operations like:
are legal in unreachable code. Unfortunately many patterns would be unsafe to apply on such IR and can lead to crashes or infinite loops. To avoid this we can remove unreachable blocks before attempting to apply patterns.
We may have to do this also whenever the CFG is changed by a pattern, it is left up for future work right now.
Fixes #153732